Conversation
pmowrer
left a comment
pmowrer
left a comment
There was a problem hiding this comment.
Thanks for the PR! I think a flag for enabling this behavior is a good idea & compromise since it appears to be a common use case.
| return parseList(value); | ||
| } else if (_.isPlainObject(value)) { | ||
| return parseMap(value); | ||
| } else if (ESCAPE_STRINGS && _.isString(value)) { |
There was a problem hiding this comment.
All caps as in ESCAPE_STRINGS usually denotes a constant, which ESCAPE_STRINGS isn't in this case. IMO, it'd be clearer (and simpler) to just use this.options.escapeStrings directly.
There was a problem hiding this comment.
Ah, while updating my PR I've realized why I've used the variable as intermediate: this is undefined in parseValue.
There was a problem hiding this comment.
Is there a better way than to put e.g. var options; in the outside scope and use that in parseValue? 😄
There was a problem hiding this comment.
Great point! I think the option should be passed to parseValue to keep that function pure.
export function parseValue(value, { escapeStrings = false }) {
...| expect(result.css.toString()).to.eql(EXPECTATION); | ||
| }); | ||
|
|
||
| it('escapes strings when escapeStrings option is set', function() { |
There was a problem hiding this comment.
When the word "when" is used in the test description, it's usually a sign that part of it can be moved into a describe as grouping mechanism. It allows for adding multiple tests for the same condition. E.g.:
describe('when escapeStrings options is set', function() {
it('escapes string values', function() {
...
});
it('does something else', function() {
...
});
});| expect(result.css.toString()).to.eql(escapeStringsExpectation); | ||
| }); | ||
|
|
||
| it(`throws on complex JSON when escapeStrings option is not set`, function() { |
There was a problem hiding this comment.
I think "complex JSON" isn't specific enough. You could construct complex JSON that doesn't throw in this test. Something like "throws when parsing a string that isn't a valid SASS expression".
| /// @param {Map} $map - Map | ||
| /// @param {Arglist} $keys - Key chain | ||
| /// @return {*} - Desired value | ||
| @function map-deep-get($map, $keys...) { |
There was a problem hiding this comment.
I like the real-world test, but all of this SASS logic kind of obscures what's actually being tested. I think we should be able to simplify this a great deal to really get at what it is that we want to test. Review #5 for some simpler examples, like @diddledan's original example or @safareli's list of edge cases.
|
Closing due to inactivity |
|
Willing to reconsider merging if I pickup and address the PR comments? |
|
Go right ahead @esr360! Appreciate your thoughts on the matter |
|
Closing due to inactivity. |
3 participants
Hi there,
I've followed the discussion in #5 and understand the difficulty to escape strings automatically when necessary.
Unfortunately I need to parse a more complex JSON and double escaping strings is not an option as the file is used in other environments as well.
As in my case escaping all strings by default is not an issue I've added such option (by default false) and additional tests.
Would be great if you would be willing to merge and publish this on NPM, so I don't need to maintain my own fork. 😄
Thanks!